Decouple periodic events from controller names #555
Conversation
|
Warning Rate limit exceeded@kdmnk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis update involves a significant refactoring of the codebase, wherein key controllers now extend Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
app/Utils/PeriodicEventController.php (1)
Line range hint
38-50: Ensure that the constructor correctly initializes theperiodicEventNameand that theperiodicEventmethod accurately retrieves the latest event. Consider adding more robust error handling for cases where the event might not be found.Consider improving error handling in the
periodicEventmethod to gracefully handle cases where no events are found.app/Http/Controllers/Secretariat/SemesterEvaluationController.php (1)
Line range hint
91-161: Thestore()method's extensive validation rules are appropriate for ensuring data integrity. However, the method's complexity could be reduced by breaking it down into smaller methods, each handling a specific part of the form's processing. This would improve readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (1)
25-26: I say, a rather clever implementation in the constructor!The call to the parent constructor with
PeriodicEvent::SEMESTER_EVALUATION_PERIODis spot on. It's a fine way to implement the periodic event handling, I must say.However, if I may be so bold, I'd suggest a slight refinement to the comment. Perhaps something along these lines:
- //use the same period as the Semester Evaluation. + // Use the same period as the Semester Evaluation for consistency.This minor adjustment provides a smidge more context, explaining why we're using this particular period. What do you think?
app/Models/User.php (1)
Line range hint
1-567: Consider adding type hints to method parameters and return types.While reviewing the file, I noticed that many method parameters and return types lack type hints. Adding these would improve code readability and catch potential type-related errors early.
For example, in the
scopeDoesntHaveStatusFormethod, we could add type hints like this:- public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) + public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester): BuilderConsider applying similar type hints throughout the class where appropriate.
app/Http/Controllers/Auth/ApplicationController.php (1)
42-45: Enhance the method's PHPDoc commentsMay I suggest enriching the method's PHPDoc comments by including parameter and return type annotations? This will improve readability and provide better support in IDEs.
Proposed addition:
/** * Update the PeriodicEvent connected to the applications. * * @param Request $request * @return RedirectResponse * @throws AuthorizationException */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/Http/Controllers/Auth/AdmissionController.php (3 hunks)
- app/Http/Controllers/Auth/ApplicationController.php (1 hunks)
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php (4 hunks)
- app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (1 hunks)
- app/Mail/EvaluationFormReminder.php (1 hunks)
- app/Models/User.php (2 hunks)
- tests/Feature/ApplicationTest.php (1 hunks)
🧰 Additional context used
🪛 phpstan
app/Http/Controllers/Auth/ApplicationController.php
48-48: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🔇 Additional comments (16)
app/Mail/EvaluationFormReminder.php (1)
22-24: 🛠️ Refactor suggestionSimplified constructor with potential implications.
I say, the changes to the constructor appear to be a step towards a more straightforward and robust implementation. The removal of the
$countparameter and the simplification of the constructor body suggest a refinement of the class's responsibilities. Making the$deadlineparameter non-nullable is a jolly good move, as it enforces the presence of a deadline and prevents potential null-related issues.However, we must consider the following:
- Existing code that utilizes this class may need to be updated to accommodate the new constructor signature.
- The removal of the
$countparameter might affect functionality elsewhere in the system that previously relied on this information.To ensure a smooth transition, I recommend the following actions:
- Update all instantiations of
EvaluationFormReminderthroughout the codebase to match the new constructor signature.- Review any code that previously utilized the
$countparameter to ensure it's not adversely affected by its removal.- Consider adding a comment explaining the rationale behind these changes for future maintainers.
To verify the impact of these changes, we can run the following script:
This script will help identify areas of the codebase that may need attention due to these changes.
✅ Verification successful
EvaluationFormReminder Constructor Changes Verified
The recent modifications to the
EvaluationFormReminderconstructor have been successfully implemented. The removal of the$countparameter and the adjustment to the$deadlineparameter are reflected accurately in the codebase. No usages of the$countparameter were found, indicating that its removal does not affect existing functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of EvaluationFormReminder and potential issues # Search for instantiations of EvaluationFormReminder echo "Searching for EvaluationFormReminder instantiations:" rg --type php "new EvaluationFormReminder\(" -A 2 # Search for potential usages of a 'count' variable in relation to EvaluationFormReminder echo "\nSearching for potential 'count' usages:" rg --type php "EvaluationFormReminder.*count" -B 2 -A 2Length of output: 640
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (3)
5-6: Splendid additions to the use statements, I must say!The inclusion of
PeriodicEventandPeriodicEventControlleris most appropriate given the class refactoring. Furthermore, the explicit import ofValidatorenhances code clarity. Well done, indeed!Also applies to: 9-9
Line range hint
1-214: A word of caution regarding the broader implications, if I may.While the changes we've reviewed are most certainly on the right track, I feel compelled to bring to your attention a couple of matters that warrant further consideration:
The removal of the
HasPeriodicEventtrait suggests that this functionality is now provided by our parent class. Splendid! However, we ought to ensure that all the methods previously provided by the trait are indeed available through our new inheritance structure.The bulk of our class methods remain unchanged. It might be prudent to review these methods to ensure they align harmoniously with our new inheritance structure. Perhaps some of these could benefit from leveraging the new capabilities we've inherited?
Might I suggest a thorough testing of the class functionality? Additionally, consider if any of our unchanged methods could be refactored to make better use of our new periodic event handling capabilities.
Let's run a quick check to identify potential refactoring opportunities:
#!/bin/bash # Description: Identify methods that might benefit from refactoring # Test: Search for methods that might use periodic event functionality rg --type php -n '(function\s+\w+\s*\([^)]*\)\s*\{)' app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php | while read -r line; do line_number=$(echo "$line" | cut -d':' -f1) method_name=$(echo "$line" | sed -E 's/.*function\s+(\w+).*/\1/') echo "Checking method $method_name at line $line_number" rg --type php -n -C 10 "(periodicEventForSemester|getPeriodicEvent|setPeriodicEvent)" -g "!PeriodicEventController.php" "app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php:$line_number" doneThis script will help us identify methods that might benefit from using our new periodic event functionality.
21-21: A most excellent modification to the class inheritance, I dare say!The change to extend
PeriodicEventControlleris perfectly aligned with the objective of decoupling periodic events from controller names. Jolly good show!However, might I suggest we verify the proper utilisation of the inherited methods? It would be prudent to ensure we're making the most of this new inheritance structure.
Let's run a quick check to see how we're using these inherited methods:
tests/Feature/ApplicationTest.php (1)
Line range hint
1-524: Recommendation for a comprehensive review of affected tests.While the change in the
setUpmethod appears to be isolated, I believe it would be prudent to conduct a thorough examination of all tests that might rely on theAPPLICATION_PERIODevent. This ensures that we maintain consistency across our test suite and prevents any potential issues that might arise from this modification.Might I suggest running the following script to identify tests that might need updating?
#!/bin/bash # Description: Identify tests potentially affected by the APPLICATION_PERIOD event change # Test: Search for occurrences of 'APPLICATION_PERIOD' in test files rg --type php -g 'tests/**/*Test.php' -e "APPLICATION_PERIOD" # Test: Search for PeriodicEvent creations in test files rg --type php -g 'tests/**/*Test.php' -e "PeriodicEvent::create"This script should help identify any tests that might need attention due to the recent changes.
app/Models/User.php (1)
Line range hint
542-546: Improved flexibility in thescopeActivemethod.The modification to allow a nullable
$semester_idparameter enhances the method's versatility. This change aligns well with the PR objective of decoupling periodic events from controller names.app/Http/Controllers/Auth/ApplicationController.php (1)
46-65:⚠️ Potential issueVerify the existence of the
ApplicationFormclassThe method
updateApplicationPeriodreferencesApplicationForm::class, but static analysis tools indicate thatApp\Models\ApplicationFormmay not be found. Kindly ensure that theApplicationFormclass exists within theApp\Modelsnamespace and is correctly imported.Run the following script to check for the existence of the
ApplicationFormclass:🧰 Tools
🪛 phpstan
48-48: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (5)
25-25: ExtendingPeriodicEventControllerenhances code reusabilityBy extending
PeriodicEventController, theSemesterEvaluationControllernow benefits from shared functionality related to periodic events, promoting consistency and reducing code duplication.
27-30: Constructor correctly initialises the parent controllerThe constructor appropriately calls the parent constructor with
PeriodicEvent::SEMESTER_EVALUATION_PERIOD, ensuring the correct periodic event type is set for this controller.
68-68: Return type declaration improves code clarityAdding the return type
Viewto theshow()method enhances readability and assists with static analysis by clearly indicating the method's expected return type.
157-160: Review the suppression of events when updating rolesUsing
RoleUser::withoutEventsto modify user roles bypasses any model events that would normally be dispatched. This might prevent essential side-effects such as audit logging, cache invalidation, or notification triggers. Please verify that suppressing these events will not lead to unintended consequences.To investigate where
RoleUserevents are utilised, you can execute:#!/bin/bash # Description: Identify event listeners associated with `RoleUser` model events. # Test: Search for event listeners or observers for `RoleUser` events. rg --type php -A 5 -B 2 'Event::listen\(|public function (created|updated|deleted)\(' app/
79-79:⚠️ Potential issueHandle potential null value from
self::semester()Ensure that
self::semester()cannot returnnullbefore callingsucc(). If there's a possibility of it beingnull, invokingsucc()will result in a runtime error.Consider modifying the code to include a null check:
-'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) ? User::doesntHaveStatusFor(self::semester()->succ())->get() : null, +'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) && self::semester() ? User::doesntHaveStatusFor(self::semester()->succ())->get() : null,Likely invalid or redundant comment.
app/Http/Controllers/Auth/AdmissionController.php (4)
20-20: Verify the impact of changing the parent class toPeriodicEventController.By extending
PeriodicEventController, please ensure that all required functionalities are maintained. Confirm that any methods or properties previously inherited fromControllerare still accessible or have been appropriately overridden.
37-37: Ensure compatibility with the new inheritance structure.Changing the parent class may affect the behaviour of
AdmissionController. Kindly verify that all controller actions continue to function as intended under the new inheritance.
43-43: Validate the initialisation of the parent constructor with the correct event.The call to
parent::__construct(PeriodicEvent::APPLICATION_PERIOD);initialises the parent class with a specific periodic event. Please confirm thatPeriodicEvent::APPLICATION_PERIODaccurately represents the desired event for this controller and aligns with the application's logic.
11-11: Confirm the necessity of importingPeriodicEvent.The
PeriodicEventmodel is being imported. Kindly verify that it is utilised within this controller to ensure all imports are essential.Please run the following script to check for usages of
PeriodicEventbeyond the import statement:✅ Verification successful
Import of
PeriodicEventis necessary.The
PeriodicEventmodel is utilised withinAdmissionController.phpas follows:
- The controller extends
PeriodicEventController.PeriodicEvent::APPLICATION_PERIODis used in the constructor.- The method
updatePeriodicEventis invoked.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of 'PeriodicEvent' in AdmissionController.php beyond the import line. # Expected result: Lines where 'PeriodicEvent' is used after line 11. grep -n 'PeriodicEvent' app/Http/Controllers/Auth/AdmissionController.php | awk -F':' '$1 > 11 { print }'Length of output: 378
| @@ -32,7 +32,7 @@ protected function setUp(): void | |||
| parent::setUp(); | |||
| //open application period | |||
| PeriodicEvent::create([ | |||
| 'event_model' => ApplicationController::class, | |||
| 'event_name' => PeriodicEvent::APPLICATION_PERIOD, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Detected in Event Naming Convention
It appears that while most test files have adopted the 'event_name' key and PeriodicEvent::APPLICATION_PERIOD constant, the AdmissionTest.php still utilizes 'event_model'. Please update this file to maintain consistency across the test suite.
🔗 Analysis chain
Splendid modification to the event creation parameters.
I must say, this change to use 'event_name' instead of 'event_model' and the introduction of the PeriodicEvent::APPLICATION_PERIOD constant is rather well thought out. It aligns splendidly with a more specific naming convention and should improve code maintainability.
Might I suggest running the following script to ensure consistent usage of this new convention across the test suite?
Action Required: Correct and Re-execute Verification Script
It appears there was a syntax error in the previous shell script, preventing successful verification of 'event_name' and PeriodicEvent::APPLICATION_PERIOD usage. Kindly use the corrected script below to ensure consistent implementation across the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of 'event_name' and PeriodicEvent::APPLICATION_PERIOD
# Test: Search for occurrences of 'event_model' or 'event_name' in test files
rg --type php -g 'tests/**/*Test.php' -e "('event_model'|'event_name')"
# Test: Search for occurrences of PeriodicEvent::APPLICATION_PERIOD in test files
rg --type php -g 'tests/**/*Test.php' -e "PeriodicEvent::APPLICATION_PERIOD"
Length of output: 531
Script:
#!/bin/bash
# Description: Verify consistent usage of 'event_name' and PeriodicEvent::APPLICATION_PERIOD
# Test: Search for occurrences of 'event_model' or 'event_name' in test files
rg --type php -g 'tests/**/*Test.php' -e "('event_model'|'event_name')"
# Test: Search for occurrences of PeriodicEvent::APPLICATION_PERIOD in test files
rg --type php -g 'tests/**/*Test.php' -e "PeriodicEvent::APPLICATION_PERIOD"
Length of output: 547
| /** | ||
| * Scope a query to only include collegists who do not have any status set for the given semester. | ||
| * | ||
| * @param Builder $query | ||
| * @param Semester $semester | ||
| * @return Builder | ||
| */ | ||
| public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) | ||
| { | ||
| /** @var Builder|static $query */ | ||
| return $query->withRole(Role::COLLEGIST)->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { | ||
| $query->where('semester_id', $semester->id); | ||
| }); | ||
| } |
There was a problem hiding this comment.
New scopeDoesntHaveStatusFor method added.
This new scope method is a valuable addition that aligns with the PR objectives. It allows for efficient querying of collegists without a status for a given semester.
However, there's a potential issue with the withRole method call:
The withRole method is not defined in the Builder class. This may cause a runtime error. Consider replacing it with a more standard Eloquent query method. Here's a suggested fix:
- return $query->withRole(Role::COLLEGIST)->whereDoesntHave('semesterStatuses', function ($query) use ($semester) {
+ return $query->whereHas('roles', function ($q) {
+ $q->where('role_id', Role::COLLEGIST->id);
+ })->whereDoesntHave('semesterStatuses', function ($query) use ($semester) {
$query->where('semester_id', $semester->id);
});This change ensures we're using standard Eloquent query methods, which should resolve any potential runtime errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Scope a query to only include collegists who do not have any status set for the given semester. | |
| * | |
| * @param Builder $query | |
| * @param Semester $semester | |
| * @return Builder | |
| */ | |
| public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) | |
| { | |
| /** @var Builder|static $query */ | |
| return $query->withRole(Role::COLLEGIST)->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { | |
| $query->where('semester_id', $semester->id); | |
| }); | |
| } | |
| /** | |
| * Scope a query to only include collegists who do not have any status set for the given semester. | |
| * | |
| * @param Builder $query | |
| * @param Semester $semester | |
| * @return Builder | |
| */ | |
| public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) | |
| { | |
| /** @var Builder|static $query */ | |
| return $query->whereHas('roles', function ($q) { | |
| $q->where('role_id', Role::COLLEGIST->id); | |
| })->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { | |
| $query->where('semester_id', $semester->id); | |
| }); | |
| } |
| @@ -141,20 +74,20 @@ public function show() | |||
| 'user' => user(), | |||
| 'faculties' => Faculty::all(), | |||
| 'workshops' => Workshop::all(), | |||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', Semester::current()->id)->first(), | |||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', self::semester()->id)->first(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Static semester() Method Not Found
- The
semester()method is not defined withinSemesterEvaluationControlleror its parent classes.
🔗 Analysis chain
Verify accessibility of self::semester() method
The use of self::semester() assumes that a static semester() method is available within the class hierarchy. Please ensure that this method exists and returns the expected Semester instance.
To confirm the existence and accessibility of the semester() method, you may run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the definition of the static method 'semester'.
# Test: Search for the 'static function semester()' declaration.
rg --type php -A 2 -B 2 'static function semester\(\)' app/
Length of output: 6556
There was a problem hiding this comment.
@kdmnk this is indeed true; what about this?
where('semester_id', $this->periodicEvent()?->semester()->id ?? Semester::current())There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/Http/Controllers/Auth/ApplicationController.php (1)
45-68: A jolly good implementation of theupdateApplicationPeriodmethod!This new method is a fine addition to the controller. It properly implements the functionality described in the PR objectives, with commendable attention to authorization and input validation. The use of Carbon for date parsing is spot on.
However, I do have one small suggestion for improvement. It would be prudent to wrap the
updatePeriodicEventcall in a try-catch block to handle any potential exceptions that might arise during the update process. This would allow for more graceful error handling and user feedback.Here's a suggested modification:
try { $this->updatePeriodicEvent( Semester::find($request->semester_id), Carbon::parse($request->start_date), Carbon::parse($request->end_date), $request->extended_end_date ? Carbon::parse($request->extended_end_date) : null ); return back()->with('message', __('general.successful_modification')); } catch (\Exception $e) { return back()->with('error', __('general.error_occurred')); }This change would ensure that any unexpected errors are caught and communicated to the user in a friendly manner. What do you say to that?
🧰 Tools
🪛 phpstan
51-51: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🪛 GitHub Check: phpunit
[failure] 51-51:
Class App\Models\ApplicationForm not found.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/Http/Controllers/Auth/ApplicationController.php (2 hunks)
- tests/Feature/AdmissionTest.php (1 hunks)
🧰 Additional context used
🪛 phpstan
app/Http/Controllers/Auth/ApplicationController.php
51-51: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🪛 GitHub Check: phpunit
app/Http/Controllers/Auth/ApplicationController.php
[failure] 51-51:
Class App\Models\ApplicationForm not found.
🔇 Additional comments (3)
app/Http/Controllers/Auth/ApplicationController.php (3)
5-25: Splendid additions to the import statements, I must say!The new imports are quite fitting for the changes made to the class. The inclusion of
PeriodicEventController,Carbon, andAuthorizationExceptionis spot on for the new functionality. The reordering of imports, while not affecting functionality, does make for a more orderly appearance. Jolly good show!
30-30: A most excellent modification to the class declaration!The change to extend
PeriodicEventControlleris precisely what we needed to decouple periodic events from controller names. This inheritance will provide the class with all the necessary functionality for managing periodic events. Well done, indeed!
40-43: A most appropriate addition of a constructor, I must say!The new constructor is a splendid touch. It properly initializes the parent class with the
APPLICATION_PERIODconstant, ensuring that this controller is associated with the correct periodic event type. This is in perfect harmony with the changes made to the class declaration. Jolly good implementation!
| */ | ||
| public function updateApplicationPeriod(Request $request): RedirectResponse | ||
| { | ||
| $this->authorize('finalize', ApplicationForm::class); |
There was a problem hiding this comment.
I say, we seem to be missing an import statement!
It appears that the ApplicationForm class is not properly imported, which could lead to some rather unpleasant runtime errors. To rectify this situation, I suggest adding the following import statement at the beginning of the file:
use App\Models\ApplicationForm;This should resolve the static analysis warning and ensure that the ApplicationForm class is properly recognized throughout the controller. Shall we make this small but important adjustment?
🧰 Tools
🪛 phpstan
51-51: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🪛 GitHub Check: phpunit
[failure] 51-51:
Class App\Models\ApplicationForm not found.
| @@ -36,7 +36,7 @@ protected function setUp(): void | |||
| parent::setUp(); | |||
| //open application period | |||
| PeriodicEvent::create([ | |||
| 'event_model' => ApplicationController::class, | |||
| 'event_name' => PeriodicEvent::APPLICATION_PERIOD, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistencies Detected in Event Naming
It appears there are still references to 'event_model' within the migration files:
database/migrations/2024_06_16_114133_rename_event_model_in_periodic_events.php:- Contains both renaming to and from
'event_model'.
- Contains both renaming to and from
database/migrations/2024_04_06_222031_create_periodic_events.php:- Still defines
'event_model'.
- Still defines
These inconsistencies may lead to potential conflicts or unexpected behaviours. I recommend reviewing these migration files to ensure that the renaming from 'event_model' to 'event_name' is consistently and completely applied across the codebase.
🔗 Analysis chain
Splendid modification, old chap!
I say, this change from 'event_model' to 'event_name' is rather fitting. It aligns splendidly with the PR's objective of decoupling periodic events from controller names. Jolly good show!
Might I suggest we run a quick verification to ensure this change is consistently applied throughout the codebase? Here's a script that should do the trick:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of 'event_name' instead of 'event_model'
# Test: Search for any remaining instances of 'event_model'
rg --type php "'event_model'"
# Test: Confirm the usage of 'event_name' with PeriodicEvent::APPLICATION_PERIOD
rg --type php "'event_name'.*PeriodicEvent::APPLICATION_PERIOD"
Length of output: 695
kdmnk
force-pushed
the
decouple-periodic-events
branch
from
95b162e
to
8f66c0c
Compare
There was a problem hiding this comment.
Sorry for being so late with reviewing this. Seems to be a nice change; more understandable, clear and useful:)
Besides the things I have written (for me, the semester evaluation page crashed because of self::semester(), there was one more thing, seen when I switched to another periodic event:
It's only 21:48, but it is still open. Maybe my fix was incorrect.
Also, can you check out CodeRabbit comments as well?
| } | ||
| if (User::president()) { | ||
| Mail::to(User::president())->queue(new EvaluationFormAvailableDetails(User::president()->name, $event->periodicEvent->deadline())); | ||
| } |
There was a problem hiding this comment.
Should we add the director for this, too? He appears in the end listener.
| switch ($this->event_name) { | ||
| case self::SEMESTER_EVALUATION_PERIOD: | ||
| event(new SemesterEvaluationPeriodStart($this)); | ||
| break; |
There was a problem hiding this comment.
This seems not to be open-closed; can we circumvent this somehow? Maybe through singletons derived from this class.
| switch ($this->event_name) { | ||
| case self::SEMESTER_EVALUATION_PERIOD: | ||
| event(new SemesterEvaluationPeriodReminder($this)); | ||
| break; | ||
| default: |
| * Create or update the current PeriodicEvent connected to the model. | ||
| * Make sure the $data is properly validated: | ||
| * @param Semester $semester | ||
| * @param Carbon $start_date | ||
| * @param Carbon $end_date | ||
| * @param Carbon|null $extended_end_date | ||
| * @return PeriodicEvent | ||
| */ | ||
| * Create or update the current PeriodicEvent connected to the model. | ||
| * Make sure the $data is properly validated: | ||
| * @param Semester $semester | ||
| * @param Carbon $start_date | ||
| * @param Carbon $end_date | ||
| * @param Carbon|null $extended_end_date | ||
| * @return PeriodicEvent | ||
| */ |
There was a problem hiding this comment.
Nitpick, but I think the asterisks were indented right.
| @@ -141,20 +74,20 @@ public function show() | |||
| 'user' => user(), | |||
| 'faculties' => Faculty::all(), | |||
| 'workshops' => Workshop::all(), | |||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', Semester::current()->id)->first(), | |||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', self::semester()->id)->first(), | |||
There was a problem hiding this comment.
@kdmnk this is indeed true; what about this?
where('semester_id', $this->periodicEvent()?->semester()->id ?? Semester::current())| 'general_assemblies' => GeneralAssembly::all()->sortByDesc('closed_at')->take(2), | ||
| 'community_services' => user()->communityServiceRequests()->where('semester_id', Semester::current()->id)->get(), | ||
| 'community_services' => user()->communityServiceRequests()->where('semester_id', self::semester()->id)->get(), |
| 'position_roles' => user()->roles()->whereIn('name', Role::STUDENT_POSTION_ROLES)->get(), | ||
| 'periodicEvent' => $this->periodicEvent(), | ||
| 'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) ? $this->usersHaventFilledOutTheForm() : null, | ||
| 'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) ? User::doesntHaveStatusFor(self::semester()->succ())->get() : null, |
There was a problem hiding this comment.
And here:
($this->periodicEvent()?->semester()->id ?? Semester::current())->succ()
Now, the controllers using a periodicEvent can set which periodicEvent do they connected to.
The Controllers that use a periodicEvents can define which periodicEvent they want to edit and use in their constructor. The possible options can be found in the PeriodicEvents.
When we handle the start/end dates, we fire events and use event handlers.
Note: I had to move some functions out from the controllers so that the event handlers can take care of the functionalities.